https://youtu.be/8hkrahP9yiY

I have to fix some soft-takeover issues too

https://youtu.be/Sb13mw06zUM

I am doubtful we should merge this. I do not want to maintain this hacky mixing of C++ and JS for tests indefinitely or provide a misleading example for future mapping developers.

would you prefer I submit the mapping with no tests? Can you be more specific about why you think this would be a maintenance burden? We already have controllerengine_test.cpp which does many of the same things. You've said you think I should wait for the new HID controller API, but that seems to be a non-sequitor -- these tests are quite basic and can be written for whatever new controller API is invented. Furthermore, the mapping will happen to be rewritten anyway. I spent two weeks trying to write this effect interface, which you asked for, and needed to write a testing framework just to make it tractable. If you're going to throw all that work out, I think you owe me a more detailed list of objections. Can you justify your position in less confrontational language ("hacky", "misleading")? Why is it bad if other people write C++ tests for their controller scripts?

My argument for why it should be included:

• the interface between c++ and js is clean: I am providing simple inputs into the js (field Objects), and testing the values or the resulting output.
• The testing is independent of the HID API -- for instance I mock out the lights API. I am testing my controller script, not the API
• Development time was greatly reduced thanks to these tests
• It will catch regressions, and has allowed me to make updates to the mapping
• c++ infra ensures that the js environment is clean for every test
• by not including the tests, we're just making it more likely that there will be regressions. I will have to maintain the test in a personal branch.

Arguments against an all-js testing infrastructure, which you have proposed:

• none exists for mixxx yet -- I would have to write an entire testing infrastructure
• there is still no new HID API, and the testing infrastructure would have to depend on that API.
• js is a dangerous language with a lot of global state and side effects. Ensuring the each test has a clean JS environment would be essentially impossible.
• trying to run tests while mixxx is running means we can't have the tests run automatically with our CI environments
• trying to run tests while mixxx is running means pollution of the testing setup by arbitrary inputs from the user while the test is running, or pollution of initial state.

We have a plan which we have invested lots of time discussing over the past several months. This is not some nebulous idea for the future; we have already taken several steps towards it. The first of those, #2682, was started as #1795 two years ago. #2868 has already been merged, now #2920 is in progress refactoring the same code you are now proposing to add a novel feature to. I feel frustrated that you're asking us to review this new feature for the legacy architecture -- implemented without discussion beforehand -- instead of helping with larger communal efforts. This takes time away from those efforts and creates merge conflicts.

I spent two weeks trying to write this effect interface, which you asked for

I specifically suggested not mapping the Mixer FX buttons, again, because we already have a major refactoring (#2618) that will make the complex design for this effects mapping for 2.3 obsolete. I also feel frustrated being asked to review this while you have explicitly said you will not review that major refactoring -- also started 2 years ago -- which has been sitting waiting for review for months.

If we do not work together towards long term goals and instead just work on the path of least resistance to implement the features we personally desire without cleaning up legacy architecture, we will not make big advancements for Mixxx.

thank you for the reply, this helps me understand where our disagreement is.

refactoring the same code you are now proposing to add a novel feature to.

I don't understand what you mean here, I moved two functions from private to protected, but otherwise I have not touched the HID API. Can you explain specifically what will be hard to maintain here?

I also feel frustrated being asked to review this while you have explicitly said you will not review that major refactoring -- also started 2 years ago -- which has been sitting waiting for review for months.

The refactoring you mention is 9000 changed lines and over 200 changed files. I have tried to give evidence about why it is unreviewable at that size. Any "LGTM" will be at best a guess. (https://tekin.co.uk/2020/05/proof-your-thousand-line-pull-requests-create-more-bugs). By all measures, it should be broken up into nearly 20 pieces for us to be sure it will work. If you want my input on it we need to find a way to break it into smaller chunks. (I notice some diffs are lint reformats. Start there! I did that with the hid js file and was painless.)

If you think this PR is also too big and would like me to break it up into pieces, I could submit 1 PR for most of the controller and only map quickeffects, not the effects buttons, and then in a subsequent PR work on the effects mappings.

I agree that we need to move forward with major features, but we need to balance the new work with making sure we support new controllers -- consistently the #1 complaint from users about mixxx. I am glad we have a plan to rework controller scripts and that work has been progressing. I'm sorry that I haven't been able to keep up with that. I am willing to take on the burden of maintaining this controller config, even if there are merge conflicts with the new incoming work. (I have also started working on updating the mapping for the VCI400, now that I understand how the standard effect mapping is supposed to operate).

I also think the testing framework I've started is valuable and we should discuss how to include it, or if you have a concrete proposal for a different controller testing framework, we should discuss it elsewhere. If you have specific objections to how the test file is written, I still don't know what they are.

So my proposal is:

• create a basic mapping for 2.3 that leaves effects unmapped
• submit the same mapping to trunk
• hash out the controller testing idea
• create a PR for the effects work in trunk, with tests

I don't understand what you mean here, I moved two functions from private to protected, but otherwise I have not touched the HID API. Can you explain specifically what will be hard to maintain here?

Now that you have reimplemented this on the master branch, I see the changes to ControllerEngine are small. However, the ControllerEngine class no longer exists in #2920.

There's no need to remove the work you've already done for this mapping from 2.3. We can merge the full mapping to 2.3 but simply not merge the tests. I do not think it is a good idea to require writing C++ to test controller scripts; IMO requiring script authors to write C++ defeats the purpose of using JS for controller mappings.

The refactoring you mention is 9000 changed lines and over 200 changed files. I have tried to give evidence about why it is unreviewable at that size. Any "LGTM" will be at best a guess. (https://tekin.co.uk/2020/05/proof-your-thousand-line-pull-requests-create-more-bugs). By all measures, it should be broken up into nearly 20 pieces for us to be sure it will work. If you want my input on it we need to find a way to break it into smaller chunks.

Merging major refactorings in small steps is often not possible unless you're okay with breaking features in master, which I am not okay with. You've had over two years to review #2618 as it was developed over 250 commits, but no one even started reviewing it until recently.

IMO requiring script authors to write C++ defeats the purpose of using JS for controller mappings.

I didn't say these sorts of tests should be required. Though it would be nice if we had more tests for controllers, indeed! I wrote the tests to aid in my own development, and without them I couldn't have gotten it working reliably. I will change the 2.3 PR to only include the mapping.

refactorings in small steps is often not possible unless you're okay with breaking features in master

I have tried several times in good faith, including linking multiple references, to work with you to develop strategies on how to break a large code change into smaller pieces without breaking master. This is a well-understood problem and not at all unusual in my professional experience. I'm sorry you haven't gotten the review attention you wanted on your large feature, but if you aren't willing to meet your reviewers halfway I'm not sure why you're surprised at the lack of engagement. If you would like my help on that PR I am still willing to do so, but it can't be as a single monolithic PR.

brought up to date with 2.3 version. (no diffs)

This is now up to date with the 2.3 version

Still failing checks. Also, we should pull 2.3 into main before merging this.

I don't think we should merge this.

Are you still concerned about the maintenance burden of the new code?

Yes. We are actively working on a new controller system. Adding novel features to the old one now doesn't make sense IMO.

Can you point out where the maintenance burden is? This is all pretty straightforward right now. It doesn't add any "new" features to the old system, it just moves some code around to expose the ability to execute js in a test. I get the desire not to pour too much work into a deprecated system, but I don't see the harm

I agree we should merge 2.3 into main and then fix any possible conflicts before merging this

Code style check fails

style issue fixed

I still think this approach to testing is not a good idea and an unnecessary maintenance burden and we should not merge it.

Can you be more specific about what maintenance burden you mean? The changes to common code are minimal and the test ensures that the s3 javascript won't rot. Tests should reduce maintenance burden, not increase it.

I am opposed to adding any new feature to legacy controller system right now. Maybe it wouldn't be that difficult to maintain when refactoring, but that isn't really predictable in advance. Moreover, the super complicated effects mapping that those test were written for will be obsoleted soon by #2618 anyway.

This PR is marked as stale because it has been open 90 days with no activity.

@ywwg what is the status of this? This adds another controller mapping, but I thought we already added it in 2.3. Can you elaborate?

Judging from the commit messages, there are also broken commits. If so, can you rebase?

This commit is the same, but it added test support for the mapping to confirm that the complex state machine works. Be objected to including the test on the grounds that it would be adding infrastructure to the deprecated controller system. So I committed the mapping in 2.3 without any tests. I don't think the overhead of the test code is that bad, or creates a maintenance burden for the deprecated system, so I'd like to still commit this. I can clean it up if you agree.

I'm worried that the commit shows up here. It should be already in main and it blows up the diff.

squashed!